Replace CHECK() with CHECK_*() so that when a check is failing, more … #2847

nwangtw · 2018-04-03T23:50:02Z

so that when a check is failing, more useful information is logged.

This PR affects only the CHECK() calls in cpp files.

…useful information is logged

Yaliang

Since you are looking into those logging messages to be informative. Would you update each check with a descriptive message?

CHECK_EQ(a, b) << ": some crazy thing is happening"

Also give a before|after example will also be useful for reviews

nwangtw · 2018-04-04T17:36:51Z

Yeah. It is a good point.

I was changing the safe ones only in this PR to be quick. For the CHECK_EQ() << "...." case I need to do some tests to verify the behavior of the library. Let me see if I can find some time to run the tests.

nwangtw · 2018-04-04T18:39:49Z

Just did a test. Seems the checks work with << as expected.

Run 1:
int a = 1, b = 2;
CHECK(a > b) << "Failing check 'a > b'";

The output is:
Check failed: a > b Failing check 'a > b'

Run 2:
int a = 1, b = 2;
CHECK_GT(a, b) << "Failing check 'gt(a,b)'";

The output is:
Check failed: a > b (1 vs. 2) Failing check 'gt(a, b)'

Let me update the checks with << operator then.

Yaliang · 2018-04-04T18:44:40Z

Just pass you a document for glog: http://rpg.ifi.uzh.ch/docs/glog.html

nwangtw · 2018-04-04T18:44:49Z

And I will see what description I can add now.

nwangtw · 2018-04-04T18:45:26Z

Cool. Thx. @Yaliang

nwangtw · 2018-04-05T03:05:58Z

Updated descriptions

Yaliang

There is a logic change in the code, some incorrect message I believe is caused copying between lines?

In addition, I would prefer the way like Failed to find xxxx instead of xxx is not found in xxx list. Check other log messages, it favors a way like (I) did something or (I'm) doing something.

Yaliang · 2018-04-05T23:24:33Z

heron/common/src/cpp/network/baseconnection.cpp

  disableRateLimit();  // To free the config object
+  bufferevent_free(buffer_);


I think it's either unrelated change or need to be rebased

Yeah. It is an unrelated change and has been merged in a different PR.

Yaliang · 2018-04-05T23:28:44Z

heron/common/src/cpp/network/client.h

@@ -245,7 +245,7 @@ class Client : public BaseClient {
      __global_protobuf_pool_release__(m);
      return;
    }
-    CHECK(m->IsInitialized());
+    CHECK(m->IsInitialized()) << "Protobuf not initialized";


Isn't m represents a Message ?

Yaliang · 2018-04-05T23:30:26Z

heron/common/src/cpp/zookeeper/zkclient.cpp

@@ -131,7 +131,7 @@ ZKClient::ZKClient(const std::string& hostportlist, EventLoop* eventLoop,
    : eventLoop_(eventLoop),
      hostportlist_(hostportlist),
      client_global_watcher_cb_(std::move(global_watcher_cb)) {
-  CHECK(client_global_watcher_cb_);
+  CHECK(client_global_watcher_cb_) << "Client global watcher is not provided";


I think it's better to explicitly say it's a callback

Yaliang · 2018-04-05T23:32:00Z

heron/instance/src/cpp/boltimpl/bolt-instance.cpp

  active_ = true;
 }

 void BoltInstance::Deactivate() {
  LOG(INFO) << "Not doing anything in Bolt Dacctivate";
-  CHECK(active_);
+  CHECK(!active_) << "Bolt instance is not active";


logic changed:

CHECK(active_) << "Bolt instance is not active"

Yaliang · 2018-04-05T23:37:05Z

heron/statemgrs/src/cpp/statemgr/heron-zkstatemgr.cpp

-                                              VCallback<> watcher) {
-  CHECK(watcher);
-  CHECK(!topology_name.empty());
+                                                   VCallback<> watcher) {


unrelated indent change

Yaliang · 2018-04-05T23:55:11Z

heron/stmgr/src/cpp/util/xor-manager.cpp

@@ -59,17 +59,17 @@ void XorManager::rotate(EventLoopImpl::Status) {
 }

 void XorManager::create(sp_int32 _task_id, sp_int64 _key, sp_int64 _value) {
-  CHECK(tasks_.find(_task_id) != tasks_.end());
+  CHECK(tasks_.find(_task_id) != tasks_.end()) << "Task " << _task_id << " is not found";


I would prefer Failed to find Task xxx

Yaliang · 2018-04-05T23:57:22Z

heron/stmgr/src/cpp/manager/stmgr.cpp

@@ -1085,7 +1085,7 @@ void StMgr::RestoreTopologyState(sp_string _checkpoint_id, sp_int64 _restore_txi
 void StMgr::StartStatefulProcessing(sp_string _checkpoint_id) {
  LOG(INFO) << "Received StartProcessing message from tmaster for "
            << _checkpoint_id;
-  CHECK(stateful_restorer_);
+  CHECK(stateful_restorer_) << "Stateful restorer doesn't exist when starting process";


maybe starting stateful process

Yaliang · 2018-04-05T23:57:59Z

heron/stmgr/src/cpp/manager/stmgr.cpp

@@ -348,7 +348,7 @@ void StMgr::CreateCheckpointMgrClient() {
 }

 void StMgr::CreateTMasterClient(proto::tmaster::TMasterLocation* tmasterLocation) {
-  CHECK(!tmaster_client_);
+  CHECK(!tmaster_client_) << "TMaster client has already existed";


TMaster Client? and also change next line Tmaster Client to TMaster Client?

Yaliang · 2018-04-05T23:58:35Z

heron/stmgr/src/cpp/manager/stmgr.cpp

@@ -282,7 +282,7 @@ void StMgr::FetchMetricsCacheLocation() {
 }

 void StMgr::StartStmgrServer() {
-  CHECK(!stmgr_server_);
+  CHECK(!stmgr_server_) << "Stmgr server is already running";


StmgrServer is already running? to keep the same way as the next line.

Yaliang · 2018-04-06T00:02:16Z

heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp

@@ -131,7 +131,7 @@ StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
 bool StMgrClientMgr::SendTupleStreamMessage(sp_int32 _task_id, const sp_string& _stmgr_id,
                                            const proto::system::HeronTupleSet2& _msg) {
  auto iter = clients_.find(_stmgr_id);
-  CHECK(iter != clients_.end());
+  CHECK(iter != clients_.end()) << "Stmgr " << _stmgr_id << " is not found in client list";


Would prefer Failed to find the client to Stmgr xxx

nwangtw · 2018-04-18T06:31:49Z

Thanks for reviewing.

This was a super quick PR and incremental improvement. It seems to be more time consuming now. I will leave this PR here for now and come back when I have the time.

Replace CHECK() with CHECK_*() so that when a check is failing, more …

d325381

…useful information is logged

Yaliang reviewed Apr 4, 2018

View reviewed changes

add check messages

c1ecdf6

Yaliang suggested changes Apr 6, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace CHECK() with CHECK_*() so that when a check is failing, more … #2847

Replace CHECK() with CHECK_*() so that when a check is failing, more … #2847

nwangtw commented Apr 3, 2018 •

edited

Loading

Yaliang left a comment

nwangtw commented Apr 4, 2018

nwangtw commented Apr 4, 2018

Yaliang commented Apr 4, 2018

nwangtw commented Apr 4, 2018

nwangtw commented Apr 4, 2018

nwangtw commented Apr 5, 2018

Yaliang left a comment

Yaliang Apr 5, 2018

nwangtw Apr 18, 2018

Yaliang Apr 5, 2018

Yaliang Apr 5, 2018

Yaliang Apr 5, 2018

Yaliang Apr 5, 2018

Yaliang Apr 5, 2018

Yaliang Apr 5, 2018

Yaliang Apr 5, 2018

Yaliang Apr 5, 2018

Yaliang Apr 6, 2018

nwangtw commented Apr 18, 2018

		disableRateLimit(); // To free the config object
		bufferevent_free(buffer_);

Replace CHECK() with CHECK_*() so that when a check is failing, more … #2847

Are you sure you want to change the base?

Replace CHECK() with CHECK_*() so that when a check is failing, more … #2847

Conversation

nwangtw commented Apr 3, 2018 • edited Loading

Yaliang left a comment

Choose a reason for hiding this comment

nwangtw commented Apr 4, 2018

nwangtw commented Apr 4, 2018

Yaliang commented Apr 4, 2018

nwangtw commented Apr 4, 2018

nwangtw commented Apr 4, 2018

nwangtw commented Apr 5, 2018

Yaliang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwangtw commented Apr 18, 2018

nwangtw commented Apr 3, 2018 •

edited

Loading